Add cargo-spatial-codegen tool #49

randomPoison · 2019-01-20T04:12:03Z

This PR replaces the setup.sh helper script used in the project-example example with a new setup subcommand in the example application itself. This provides better cross-platform support for running the example, specifically it makes it easier to run the example on Windows.

Additional details:

Added a new setup.rs module in project-example that replicates the logic in setup.sh.
Added argument parsing for a new setup subcommand in argument_parsing.rs.
Adds some additional logic in main() to handle the two different ways the example can be run (i.e. launching a worker or performing setup).
Tweaked the logic for handling errors in the command line arguments to log directly to stderr instead of panicking).

Please let me know if there's anything I need to change!

randomPoison · 2019-01-21T04:33:05Z

Looks like the travis build failed, but I'm unclear on what the error is? It seems like the environment variable SPATIAL_OAUTH is missing, any idea why my change would have caused that?

jamiebrynes7 · 2019-01-21T18:01:52Z

Ah that's because the PR is coming from a fork and the SPATIAL_OAUTH is a secret which Travis doesn't expose to PRs from forks. Will look into solutions for this 😄

jamiebrynes7 · 2019-01-21T18:09:37Z

Looks like I'll have to manually run the CI suite locally and override the failing check when we go to merge. Will take a look at the code later 👍

jamiebrynes7

I like the change from sh -> cargo command as its very idiomatic and one less language/complexity to worry about.

However, I feel that such a tool would be better placed as a standalone binary in spatialos-sdk-tools since it can be made generic for all projects (given a set of input schema directories and an output directory).

Conflating the worker binary and the setup for the worker binary feels a little strange at first glance.

If you are up for it - I'd love it if you could refactor project-example/setup.rs into a standalone binary in the spatialos-sdk-tools crate.

In the future, it would probably be wrapped into a codegen tool which would compile the schema into the descriptor as well as generate Rust code.

spatialos-sdk/examples/project-example/setup.rs

randomPoison · 2019-01-21T22:20:26Z

I agree that splitting the setup script into its own is a good idea. I've been trying to setup my own project based on the example in this repo, and I'm finding that I need this setup script in order to compile the schema files. Splitting it off means that I can install it on its own, rather than having to copy-paste the script into my project.

* Revert changes to argument_parsing.rs and update project-example. * Update README.md with new instructions for running setup. * Tweak logic in setup to not be hard coded for the project-example structure.

randomPoison · 2019-01-22T00:55:43Z

btw I'm marking this as WIP, I want to do a cleanup pass and make sure errors are being handled properly before this gets merged.

randomPoison · 2019-01-22T06:18:48Z

I've cleaned up the error handling logic (i.e. not just panicking on every error) and removed the WIP marker.

jamiebrynes7

One small nit - but mostly LGTM

spatialos-sdk-tools/src/setup/main.rs

jamiebrynes7

Nice! LGTM - will run the code through "CI" later this evening 👍

jamiebrynes7 · 2019-01-22T18:24:57Z

So running the command listed in the adjusted README gives me the following:

λ cargo run --bin setup -- spatialos-sdk/examples/project-example/spatialos/schema
   Compiling spatialos-sdk-tools v0.0.0 (C:\Workspace\personal\spatialos-sdk-rs\spatialos-sdk-tools)
    Finished dev [unoptimized + debuginfo] target(s) in 1.36s
     Running `target\debug\setup.exe spatialos-sdk/examples/project-example/spatialos/schema`
C:/Workspace\personal\spatialos-sdk-rs\dependencies\std-lib\improbable\standard_library.schema: Input file is not contained in any --schema_path. One --schema_path argument must be an exact prefix of the input file.
C:/Workspace\personal\spatialos-sdk-rs\dependencies\std-lib\improbable\vector3.schema: Input file is not contained in any --schema_path. One --schema_path argument must be an exact prefix of the input file.

Which looks like an error from the schema-compiler.

I think you've omitted the --schema-path argument seen here https://github.com/jamiebrynes7/spatialos-sdk-rs/blob/master/spatialos-sdk/examples/project-example/setup.sh#L15

EDIT: Actually you haven't. Not sure what's happening here 🤔

randomPoison · 2019-01-23T02:40:16Z

Hmmmmm, I was running into an issue similar to this before. It seems like schema-compiler requires --schema-path to be byte-for-byte identical with the leading part of any schemas you pass in. This can lead to it rejecting cases that should be valid. e.g. I ran into this on windows in cases where forward- and back-slashes got mixed up:

schema-compiler \
  --schema-path=C:\foo\bar\improbable/std-lib/schema \
  C:\foo\bar\improbable\std-lib\schema\whatever.schema

In that example --schema-path is logically a prefix of whatever.schema, however it mixes separators, which causes schema-compiler to reject the invocation.

You can actually see what is (I think) the cause in the example you posted:

C:/Workspace\personal\spatialos-sdk-rs\dependencies\std-lib\improbable\standard_library.schema

The first separator is / but the subsequent ones are \. I think the issue is in your SPATIAL_LIB_DIR variable, it probably defines the path with inconsistent separators.

Ultimately, this is an issue with shema-compiler in that it's rejecting perfectly valid paths. I can see if I can work around it for now by normalizing the paths before invoking the command.

randomPoison · 2019-01-23T02:43:44Z

I was able to reproduce the issue by using the --spatial-lib-dir argument to manually specify a path with mixed separators. I've added path normalization logic that fixes the issue in the cases that I was able to test. The logic is a bit ugly, but tbh the "right" solution is to fix schema_compiler to handle paths better. In the meantime, I've added a HACK comment clarifying exactly why the functionality is needed.

As an aside, there's a tracking issue for adding path normalization logic to std, but it hasn't landed on stable yet 😭

Generates the bundle.json file needed as part of code generation in #6.

randomPoison · 2019-01-29T23:39:11Z

@jamiebrynes7 @davedissian I've updated this to add support for codegen in the setup script. setup can now run codegen, so there's only a single step users need to run in order to prepare their project (or the example). This should be ready for review/merge whenever you're ready 🙂

I've also deleted the generated_code.rs from the example and set it up to be ignored by git. The generated file should probably not be checked into git, since it will be generated by the user as part of the setup process.

README.md

jamiebrynes7 · 2019-01-30T20:38:47Z

spatialos-sdk/examples/project-example/.gitignore

@@ -0,0 +1,2 @@
+# Generated bindings to SpatialOS components, shouldn't be committed.
+generated_code.rs


There's an argument that while codegen is under heavy development, having some example code checked in makes it a lot easier to review rather than t4 templates

I think that's a fair point, though I think it would make more sense to have small, controlled examples as part of the spatialos-sdk-code-generator crate, rather than using the example project for that purpose. That said, I'm happy to restore generated_code.rs if you'd prefer 🙂

spatialos-sdk-tools/src/setup/main.rs

jamiebrynes7 · 2019-01-30T20:51:04Z

spatialos-sdk-tools/src/setup/main.rs

+    let descriptor_out_arg = OsString::from("--descriptor_set_out=")
+        .tap(|arg| arg.push(normalize(output_dir.join("schema.descriptor"))));
+    let mut command = Command::new(&schema_compiler_path);
+    command


Can combine the above line with this block:

let mut command = Command::new(&schema_compiler_path) .arg(...) ... .arg(...);

Also a line of whitespace between the last path constructor and the command declaration would help with readability :)

Unfortunately, I don't think I can combine these. On line 87 I conditionally add arguments to command (in a loop), which means I need the command bound to a variable. But if I try to bind the return value of arg() to a variable, I'll get a compiler error, e.g.

let mut command = Command::new(...) .arg(...) .arg(...);

gives the error:

error[E0716]: temporary value dropped while borrowed --> spatialos-sdk-tools\src/codegen/main.rs:79:23 | 79 | let mut command = Command::new(&schema_compiler_path) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary which is freed while still in use ... 84 | .arg("--load_all_schema_on_schema_path"); | - temporary value is freed at the end of this statement ... 88 | command.arg(&arg); | ------- borrow later used here | = note: consider using a `let` binding to create a longer lived value

The only way to make this work is to bind the result of Command::new() to a variable, then call arg() on it in a chain.

As an aside, this is why I use the tap crate and avoid writing methods that return Self. There's no really good way to do method chaining by returning Self in Rust, and tap is the only robust solution I've found.

spatialos-sdk-tools/src/setup/main.rs

jamiebrynes7 · 2019-01-30T20:58:29Z

spatialos-sdk-tools/src/setup/main.rs

+    ///
+    /// If not specified, will not perform code generation.
+    #[structopt(long, short = "c", parse(from_os_str))]
+    codegen: Option<PathBuf>,


Maybe codegen_out_path ?

Hrmmm, so I originally had it as something like that, but changed it to --codegen because I wanted it to more clearly indicate that specifying that flag was also enabling codegen. I can change it back to --codegen-out-path if you think that's better, I don't have super strong feelings about it either way. Though, if you think --codegen is fine as the CLI flag and you just want the variable to be codegen_out_path, I can also do that 😁

jamiebrynes7 · 2019-01-30T20:58:44Z

spatialos-sdk-tools/src/setup/main.rs

+
+    /// The path the output directory for the project
+    #[structopt(long, short, parse(from_os_str))]
+    output_dir: PathBuf,


Maybe descriptor_out_dir ?

output_dir is currently used for both schema.descriptor and bundle.json. Does it make sense to call it descriptor_out_dir when it's used for both? Or should those two be split into separate flags?

spatialos-sdk-tools/src/setup/main.rs

randomPoison · 2019-01-31T19:07:25Z

Do we even have to list all the schema files when invoking schema_compiler? If I'm reading the docs correctly, the --load_all_schema_on_schema_path flag that we're passing in is already telling it to load all the schema files. If we don't need to manually pass in any files, that would allow us to simplify the code further.

randomPoison · 2019-02-01T17:19:26Z

I'm going to close this PR in favor of working on a more robust cargo-spatial tool. A PR for that should be coming soon :3

randomPoison added 7 commits January 19, 2019 13:08

Initial setup logic in project-example

2aaa589

Run the schema compiler on all schema files

14dda35

Run protoc and remove temp dir

aae6322

Simplify arguments for the setup subcommand

aa8a226

Update README.md and remove setup.sh

6b15455

Cleanup logic in main() around setup vs worker

7810b5d

Fix slashes in paths in README.md

b2b611b

jamiebrynes7 suggested changes Jan 21, 2019

View reviewed changes

randomPoison added 2 commits January 21, 2019 16:40

Make setup its own binary in spatialos-sdk-tools

3245abb

* Revert changes to argument_parsing.rs and update project-example. * Update README.md with new instructions for running setup. * Tweak logic in setup to not be hard coded for the project-example structure.

Change globbing logic to match original bash script

e705498

randomPoison changed the title ~~Add setup subcommand for project-example~~ [WIP] Add setup subcommand for project-example Jan 22, 2019

randomPoison added 2 commits January 22, 2019 00:09

Fixup error handling

9ffccf3

Cleanup spacing and comments

57bf2d0

randomPoison changed the title ~~[WIP] Add setup subcommand for project-example~~ Add setup subcommand for project-example Jan 22, 2019

Cleanup error handling in example

52f7a3d

Add clearer comments in example

e7d788e

jamiebrynes7 reviewed Jan 22, 2019

View reviewed changes

spatialos-sdk-tools/src/setup/main.rs Outdated Show resolved Hide resolved

Rename schema_dir to output_dir

9041848

jamiebrynes7 approved these changes Jan 22, 2019

View reviewed changes

Normalize path separators

f713abd

Run rustfmt

a88d8c0

randomPoison added 14 commits January 23, 2019 20:21

Generate bundle.json as part of setup

4a9b376

Generates the bundle.json file needed as part of code generation in #6.

Allow user to specify more schema paths

1a5171a

Fix #[structopt] attribute

a105262

Fix the --schema_path argument to schema_compiler

877acc2

Don't append bin to output_dir

b788212

Merge branch 'master' into example-setup-script

0c89058

Add logging to setup

0f20be4

Run code generation as part of setup

926f78f

Delete codegen.sh

d3d08b1

Make the output dir a flag, update docs

dd1275a

Remove leading ./ from paths because it trips up schema_compiler

db68932

Update docs and comments for setup

5ef7ca3

Delete and ignore generated bindings

ad94734

Fix .gitignore for generated bindings

661412d

jamiebrynes7 suggested changes Jan 30, 2019

View reviewed changes

randomPoison added 4 commits January 30, 2019 21:59

Rename setup to cargo-spatial-codegen

1cc6817

Add add_schemas helper function

5baba10

Remove invocation of protoc

73477f5

Cleanup error handling for code generation

8f2a71e

randomPoison changed the title ~~Add setup subcommand for project-example~~ Add cargo-spatial-codegen tool Jan 31, 2019

Actually check the exit status of schema_compiler

3ec7231

randomPoison closed this Feb 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cargo-spatial-codegen tool #49

Add cargo-spatial-codegen tool #49

randomPoison commented Jan 20, 2019

randomPoison commented Jan 21, 2019

jamiebrynes7 commented Jan 21, 2019

jamiebrynes7 commented Jan 21, 2019

jamiebrynes7 left a comment •

edited

Loading

randomPoison commented Jan 21, 2019

randomPoison commented Jan 22, 2019

randomPoison commented Jan 22, 2019

jamiebrynes7 left a comment

jamiebrynes7 left a comment

jamiebrynes7 commented Jan 22, 2019 •

edited

Loading

randomPoison commented Jan 23, 2019

randomPoison commented Jan 23, 2019

randomPoison commented Jan 29, 2019 •

edited

Loading

jamiebrynes7 Jan 30, 2019

randomPoison Jan 31, 2019

jamiebrynes7 Jan 30, 2019

randomPoison Jan 31, 2019

jamiebrynes7 Jan 30, 2019

randomPoison Jan 31, 2019

jamiebrynes7 Jan 30, 2019

randomPoison Jan 31, 2019

randomPoison commented Jan 31, 2019

randomPoison commented Feb 1, 2019

		@@ -0,0 +1,2 @@
		# Generated bindings to SpatialOS components, shouldn't be committed.
		generated_code.rs

Add cargo-spatial-codegen tool #49

Add cargo-spatial-codegen tool #49

Conversation

randomPoison commented Jan 20, 2019

randomPoison commented Jan 21, 2019

jamiebrynes7 commented Jan 21, 2019

jamiebrynes7 commented Jan 21, 2019

jamiebrynes7 left a comment • edited Loading

Choose a reason for hiding this comment

randomPoison commented Jan 21, 2019

randomPoison commented Jan 22, 2019

randomPoison commented Jan 22, 2019

jamiebrynes7 left a comment

Choose a reason for hiding this comment

jamiebrynes7 left a comment

Choose a reason for hiding this comment

jamiebrynes7 commented Jan 22, 2019 • edited Loading

randomPoison commented Jan 23, 2019

randomPoison commented Jan 23, 2019

randomPoison commented Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randomPoison commented Jan 31, 2019

randomPoison commented Feb 1, 2019

jamiebrynes7 left a comment •

edited

Loading

jamiebrynes7 commented Jan 22, 2019 •

edited

Loading

randomPoison commented Jan 29, 2019 •

edited

Loading